Skip to content

feat(map-efficient): #303 Slice 5b — real concurrent dispatch (flag-gated, default-off)#308

Merged
azalio merged 11 commits into
mainfrom
feat-303-slice5b-concurrent-dispatch
Jun 29, 2026
Merged

feat(map-efficient): #303 Slice 5b — real concurrent dispatch (flag-gated, default-off)#308
azalio merged 11 commits into
mainfrom
feat-303-slice5b-concurrent-dispatch

Conversation

@azalio

@azalio azalio commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Summary

#303 Slice 5b — activate REAL concurrent Actor dispatch in /map-efficient: when a wave's color group is parallel-ready, the operator emits N Task(actor) blocks in one assistant message. Flag-gated and default-OFF (execution.concurrent_dispatch=false) → byte-identical to 5a. Slice 6 (default flips) is out of scope.

Implemented per the llm-council deep verdict (conv d5ff5181, 5b round, tracked on #303), which split 5b into 7 sub-slices (5b.0–5b.6). 10 subtasks via /map-efficient; every Monitor valid/approve; independent final-verifier PASS; full make check green (3207 passed).

What changed

ST Sub-slice Change
ST-000 5b.0 execution.concurrent_dispatch (bool, default false) + max_wave_retries (int 3, [1,10]) config + clamp
ST-001 5b.0 compute_dispatch_gate — config-driven fail-closed gate in get_wave_step. Gate = concurrent_dispatch ∧ concurrency_allowed ∧ isolation≠off; flag-false short-circuits first (no probe — HC-1 byte-identity); hard-aborts (DispatchGateError) when flag on + isolation off (HC-3, never silent-degrade). WAVE_CONCURRENCY_ENABLED kept dormant.
ST-002 5b.1 Runner group-lifecycle verbs: begin_wave_group / record_group_lifecycle / verify_group_clean / reconcile_orphan_groups (idempotent, read/record-only)
ST-003 5b.2 record_dispatch_actual + classify_dispatch — coordinator-owned phantom-parallelism detector. Clock-free max_in_flight from sorted lifecycle-event replay; evidence hierarchy (worktree-SHA proves isolation, not concurrency); self-report never authoritative. Activates write_parallelism_reportparallelism.json (concurrent path only).
ST-004 5b.3 Deterministic barrier-based test harness — a threading.Barrier(N) fake Task deadlocks if the host is secretly sequential (no wall-clock); golden parsing + anti-phantom negatives.
ST-005 5b.4 run_concurrent_wave — batch-split by max_actors (now live, [1,8]); each sub-batch merged atomically via the existing merge_wave_worktrees (HC-4, all-or-nothing).
ST-006 5b.5 abort_wave_group — idempotent whole-group rollback reusing _wt_rollback (clean -fd -e .map …, never clean -fdx); run_concurrent_wave bounded restart (max_wave_retries), WAVE_RETRY_EXHAUSTED → escalate-to-human, no auto-restart.
ST-007 5b.6 Prompt integration — flip the prose-gated concurrent-fanout section from "not yet shipped" → "active on opt-in"; add the retry-discard note. Runtime-gated (dispatch_mode==concurrent), sequential default text unchanged (HC-1).
ST-008 guards HC-1 leak-guard suite — 5 non-tautological guards (fanout-token confinement, monkeypatch-fail, AST sequential-path-clean, no-telemetry, default-off baseline).
ST-009 close Docs (CHANGELOG/ARCHITECTURE/USAGE) + flip ST-000's now-stale dormancy guards to active-presence; full make check green.

Hard constraints (all proven by tests; final-verifier confirmed)

  • HC-1 behavior-neutral default (concurrent_dispatch=false → byte-identical to 5a) + a 5-guard leak suite (negative-proven).
  • HC-2 single-source render; make check-render clean; runner/orchestrator standalone (no mapify_cli import).
  • HC-3 hard-abort on explicit-enable-with-missing-prereq — never silent-degrade.
  • HC-4 GSD-style fresh-context-per-task: worktree isolation for autonomous execution #284 atomic whole-wave invariant — discard-all-or-merge-all; _wt_rollback reused, no raw clean -fdx, .map/ survives abort.
  • HC-5 concurrency proven by a barrier (deterministic), never wall-clock; no live LLM in CI.
  • HC-6 every surfaced error fixed; full make check (3207 passed) green.

Key residual risk (by design)

Same-turn N-Task emission is LLM behavior the Python layer cannot force or directly observe — green CI proves the plan, not the act. The phantom_parallel / same_turn_but_host_sequential classifier (ST-003) is the post-deployment detector and ships with 5b.

Follow-up (not in this PR)

  • Slice 6: flip concurrent_dispatch / wave_mode / worktree.isolation defaults after a soak. No shadow-mode rollout.

Part of #303. Builds on 5a (#306).

Summary by CodeRabbit

  • New Features
    • Added opt-in “concurrent Actor dispatch” within parallel waves, gated by execution.concurrent_dispatch.
    • Introduced configurable caps for concurrent actors and bounded wave retries (execution.max_wave_retries).
    • Enhanced dispatch evidence classification and concurrency reporting when true concurrency is observed.
  • Bug Fixes
    • Concurrent dispatch now fails closed when required isolation isn’t enabled.
    • If any actor in a concurrent group fails, the entire group is discarded, worktrees roll back, and the wave group is retried from the base (no partial success).
  • Documentation
    • Updated usage, architecture, and skill references to clarify Slice 5b activation and failure/retry semantics.
  • Tests
    • Added deterministic harnesses and leak-guard coverage for concurrency detection, telemetry, and rollback behavior.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds opt-in concurrent Actor dispatch for parallel waves, with config gating, wave-group lifecycle tracking, abort/retry coordination, parallelism classification, updated docs, and expanded tests.

Changes

Concurrent Actor Dispatch — Slice 5b

Layer / File(s) Summary
Config fields and dispatch gate
src/mapify_cli/config/project_config.py, src/mapify_cli/templates/map/scripts/map_orchestrator.py, .map/scripts/map_orchestrator.py, src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja
Adds concurrent_dispatch and max_wave_retries, clamps the new retry limit, and routes wave dispatch through compute_dispatch_gate with fail-closed isolation handling and stable reasons.
Wave-group lifecycle sidecar
src/mapify_cli/templates/map/scripts/map_step_runner.py, .map/scripts/map_step_runner.py, src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
Adds wave-group lifecycle state, advisory locking, clean/reconcile checks, and lifecycle event recording for concurrent wave groups.
Concurrent wave coordinator and CLI wiring
src/mapify_cli/templates/map/scripts/map_step_runner.py, .map/scripts/map_step_runner.py, src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
Adds batching, abort rollback, concurrent wave execution, and CLI entrypoints for lifecycle, abort, and telemetry commands.
Parallelism observability
src/mapify_cli/parallelism_observability.py
Adds dispatch outcome constants, deterministic classification, and gated persistence of parallelism reports.
Skill, reference, architecture, usage, and changelog docs
CHANGELOG.md, docs/ARCHITECTURE.md, docs/USAGE.md, .agents/skills/map-efficient/*, .claude/skills/map-efficient/*, src/mapify_cli/templates/*/skills/map-efficient/*
Updates the Slice 5b documentation across all copies to describe opt-in concurrent dispatch, runtime wiring, failure handling, and retry/discard behavior.
Barrier-backed fake task tool
tests/_fake_task_tool.py
Adds the barrier-based fake task tool used to prove concurrent in-flight behavior and lifecycle event recording.
Concurrent dispatch harness and integration tests
tests/test_concurrent_dispatch_harness.py
Adds barrier concurrency proofs, task-block parsing, anti-phantom classification, no-LLM guards, and real git abort integration coverage.
Config and gate tests
tests/test_project_config.py, tests/test_map_orchestrator.py
Updates config parsing/clamping coverage and gate tests for sequential, concurrent, and fail-closed cases.
Step runner lifecycle and wave tests
tests/test_map_step_runner.py
Adds lifecycle, batching, abort, retry, escalation, and lock tests for the concurrent wave runner.
Parallelism observability tests
tests/test_parallelism_observability.py
Adds classifier truth-table, replay-derived max-in-flight tests, and record-once JSON persistence tests.
Default-off leak guards
tests/test_slice5b_leak_guards.py
Adds guards that keep concurrent verbs unreachable under default configuration and verify sequential-only behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Agent as MAP Agent
  participant Orch as get_wave_step
  participant Gate as compute_dispatch_gate
  participant Runner as run_concurrent_wave
  participant Merge as merge_wave_worktrees
  participant Abort as abort_wave_group

  Agent->>Orch: get_wave_step(branch)
  Orch->>Gate: compute_dispatch_gate(branch)
  Gate-->>Orch: dispatch_mode / reason / concurrency_enabled
  Orch-->>Agent: wave step response
  Agent->>Runner: run_concurrent_wave(group_ids)
  Runner->>Merge: merge sub-batch
  alt merge fails
    Runner->>Abort: abort_wave_group(group_id)
    Abort-->>Runner: rollback verified
    Runner-->>Runner: retry or exhaust
  else merge succeeds
    Merge-->>Runner: merged ids
  end
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related issues

Possibly related PRs

Poem

🐇 I hopped through waves both wide and neat,
with Task(actor) drums and barrel feet.
One turn per batch, the group takes flight,
aborts roll back the dusk to light.
Now concurrent waves go thump and sway—
I nibble carrots, then dispatch away.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: opt-in Slice 5b concurrent dispatch with default-off gating.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-303-slice5b-concurrent-dispatch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja (1)

1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Re-render the Codex copy
src/mapify_cli/templates/skills/map-efficient/efficient-reference.md already matches the Jinja source; src/mapify_cli/templates/codex/skills/map-efficient/efficient-reference.md still diverges and should be regenerated from src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja`
at line 1, The Codex copy of the map-efficient supporting reference is out of
sync with the Jinja source. Regenerate
src/mapify_cli/templates/codex/skills/map-efficient/efficient-reference.md from
src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja,
using the same content as the matching template under
src/mapify_cli/templates/skills/map-efficient/efficient-reference.md.

Source: Coding guidelines

🧹 Nitpick comments (3)
tests/test_map_orchestrator.py (1)

5370-5436: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a mixed-wave regression for the active-wave gate.

VC3 covers “active wave is parallel” and “all waves are width-1”, but not [["ST-001"], ["ST-002", "ST-003"]] with current_wave_index=0. That gap lets a later parallel group incorrectly make the current sequential wave report concurrent dispatch.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_map_orchestrator.py` around lines 5370 - 5436, Add a regression
test for the active-wave gate where the first wave is width-1 and a later wave
is parallel, using compute_dispatch_gate and the existing WAVE_REASON_*
constants. In test_vc3_flag_on_isolation_required_concurrent_and_sequential, add
a case with execution_waves like [["ST-001"], ["ST-002", "ST-003"]] and
current_wave_index=0, then assert the result stays sequential and does not
return WAVE_REASON_CONCURRENT_GATED. Keep the focus on
map_orchestrator.compute_dispatch_gate and the wave-reason symbols so the
mixed-wave behavior is locked down.
tests/test_concurrent_dispatch_harness.py (1)

54-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Avoid leaking generated-script precedence into the whole test session.

sys.path.insert(0, ...) at module import time can change how later tests resolve map_step_runner. Load the generated runner with a temporary path restore or importlib under a private module name.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_concurrent_dispatch_harness.py` around lines 54 - 59, The
module-level sys.path.insert(0, ...) in test_concurrent_dispatch_harness.py is
leaking generated-script precedence into the entire test session. Update the
import setup around _SCRIPTS_PATH and the map_step_runner import to use a
temporary path change or importlib-based loading under a private module name,
then restore sys.path immediately after loading so later tests do not resolve
map_step_runner from the generated scripts path.
tests/test_map_step_runner.py (1)

13609-13614: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise retry tests with destructive abort semantics.

The no-op abort_wave_group stubs let retry assertions pass even though the real abort deletes worktrees and group sidecar state. Add one retry test that uses real abort behavior or a fake that removes retry inputs.

Also applies to: 14035-14041

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_map_step_runner.py` around lines 13609 - 13614, The retry coverage
in map_step_runner is still using a no-op abort stub, so the test does not
reflect the destructive behavior of abort_wave_group. Update the retry test
around _fake_abort (and the related retry case) to use the real abort path or a
fake that actually removes the retry inputs/worktree state, then assert the
retry behavior with that cleanup in place. Keep the existing merge failure setup
via _fake_merge_fail and the retry assertions, but ensure the abort semantics
are exercised realistically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/mapify_cli/parallelism_observability.py`:
- Around line 205-208: The `phantom_parallel` branch in
`parallelism_observability.py` is too eager: `skill_reported_concurrent` with
`same_turn_task_count <= 1` should only map to
`DISPATCH_OUTCOME_PHANTOM_PARALLEL` when there is no conflicting in-flight
evidence. Update the Rule 4 check in the dispatch classification logic to also
require `max_in_flight < 2` (or otherwise defer to the Rule 6 `unknown` fallback
when in-flight evidence contradicts the self-report), keeping the behavior
aligned with the existing `skill_reported_concurrent`, `same_turn_task_count`,
and `max_in_flight` rules.

In `@src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja`:
- Around line 2636-2649: The active wave should determine dispatch mode, but the
current logic in get_wave_step() relies only on
select_execution_strategy(branch, project_dir), which can mark
concurrency_allowed true based on any later color group. Update this decision to
inspect the current wave’s own parallelizability before returning
dispatch_mode="concurrent" or concurrency_enabled=True, and keep sequential when
the active wave width is 1 even if another wave is parallel.

In `@src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja`:
- Around line 19103-19118: The classifier currently only receives the
group-level base_sha, so mixed worktree bases never trigger the isolation check
in classify_dispatch(). Update the base SHA collection in the step runner to
gather each subtask’s worktree base SHA from the per-subtask records instead of
only _group_data["base_sha"], and keep feeding that list into classify_dispatch
so len(set(base_shas)) can detect mixed bases correctly.
- Around line 17448-17458: The retry path in merge_wave_worktrees is using the
same batch IDs after abort_wave_group has already deleted the group worktrees
and removed the wave_groups entry, so the next attempt has no inputs to merge.
Update the failure handling in the sub_batches loop to avoid retrying with
destroyed inputs: either perform the retry before calling abort_wave_group,
re-dispatch/recreate the actor worktrees before the next merge attempt, or abort
once and propagate the failure. Use merge_wave_worktrees, abort_wave_group, and
the failed/batches_merged bookkeeping to keep the retry flow consistent.
- Around line 15956-15992: Serialize the lifecycle update flow in the worktree
state helper so `seq` is assigned under a single writer lock instead of after a
plain read in the current lifecycle append path. In the block that reads
`state`, mutates `wave_groups[group_key]["lifecycle"]`, computes `max_seq`, and
calls `_write_worktree_state`, add synchronization around the read-modify-write
sequence to prevent two callers from reusing the same sequence number. Keep the
fix localized to the lifecycle event update logic and preserve the existing
monotonic `seq` behavior in the same function.
- Around line 17429-17439: Gate run_concurrent_wave() with
_concurrent_dispatch_enabled() before any batching or merge work so the
default-off execution.concurrent_dispatch config is respected. Add an early
return or error in the run_concurrent_wave flow, near the existing
branch_name/ids_sorted setup, when _concurrent_dispatch_enabled(pd) is false,
and keep the behavior consistent for direct CLI/coordinator calls. Use the
existing symbols run_concurrent_wave, _concurrent_dispatch_enabled, and
_wt_error to locate and implement the check.
- Around line 16115-16124: The terminal-check in the lifecycle sweep is too
permissive because the all(...) expression in the subtask completion logic only
evaluates list-valued lifecycle entries and skips missing or non-list slots.
Update the terminal-event validation in the map_step_runner template so every
declared subtask must be present and have at least one terminal event before
marking the group as terminal, using the existing lifecycle.values() /
all_terminal logic as the place to tighten the check.
- Around line 17338-17358: The abort/cleanup flow in the worktree rollback path
is removing the group entry even when rollback or discard operations may have
failed, which can hide an incorrect HEAD state. In the cleanup logic around
_wt_rollback(base_sha), discard_subtask_worktree, and record_group_lifecycle,
first capture and check the success/failure of rollback and branch cleanup, and
only delete the wave_groups entry after a verified successful rollback to
base_sha. If rollback fails, preserve the group state (including base_sha) so
verify_group_clean can detect the inconsistency.

---

Outside diff comments:
In
`@src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja`:
- Line 1: The Codex copy of the map-efficient supporting reference is out of
sync with the Jinja source. Regenerate
src/mapify_cli/templates/codex/skills/map-efficient/efficient-reference.md from
src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja,
using the same content as the matching template under
src/mapify_cli/templates/skills/map-efficient/efficient-reference.md.

---

Nitpick comments:
In `@tests/test_concurrent_dispatch_harness.py`:
- Around line 54-59: The module-level sys.path.insert(0, ...) in
test_concurrent_dispatch_harness.py is leaking generated-script precedence into
the entire test session. Update the import setup around _SCRIPTS_PATH and the
map_step_runner import to use a temporary path change or importlib-based loading
under a private module name, then restore sys.path immediately after loading so
later tests do not resolve map_step_runner from the generated scripts path.

In `@tests/test_map_orchestrator.py`:
- Around line 5370-5436: Add a regression test for the active-wave gate where
the first wave is width-1 and a later wave is parallel, using
compute_dispatch_gate and the existing WAVE_REASON_* constants. In
test_vc3_flag_on_isolation_required_concurrent_and_sequential, add a case with
execution_waves like [["ST-001"], ["ST-002", "ST-003"]] and
current_wave_index=0, then assert the result stays sequential and does not
return WAVE_REASON_CONCURRENT_GATED. Keep the focus on
map_orchestrator.compute_dispatch_gate and the wave-reason symbols so the
mixed-wave behavior is locked down.

In `@tests/test_map_step_runner.py`:
- Around line 13609-13614: The retry coverage in map_step_runner is still using
a no-op abort stub, so the test does not reflect the destructive behavior of
abort_wave_group. Update the retry test around _fake_abort (and the related
retry case) to use the real abort path or a fake that actually removes the retry
inputs/worktree state, then assert the retry behavior with that cleanup in
place. Keep the existing merge failure setup via _fake_merge_fail and the retry
assertions, but ensure the abort semantics are exercised realistically.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b0eecf5b-0e6b-4ae6-bc66-58088f203ac8

📥 Commits

Reviewing files that changed from the base of the PR and between 26490c3 and 49ac53f.

📒 Files selected for processing (30)
  • .agents/skills/map-efficient/SKILL.md
  • .agents/skills/map-efficient/efficient-reference.md
  • .claude/skills/map-efficient/SKILL.md
  • .claude/skills/map-efficient/efficient-reference.md
  • .map/scripts/map_orchestrator.py
  • .map/scripts/map_step_runner.py
  • CHANGELOG.md
  • docs/ARCHITECTURE.md
  • docs/USAGE.md
  • src/mapify_cli/config/project_config.py
  • src/mapify_cli/parallelism_observability.py
  • src/mapify_cli/templates/codex/skills/map-efficient/SKILL.md
  • src/mapify_cli/templates/codex/skills/map-efficient/efficient-reference.md
  • src/mapify_cli/templates/map/scripts/map_orchestrator.py
  • src/mapify_cli/templates/map/scripts/map_step_runner.py
  • src/mapify_cli/templates/skills/map-efficient/SKILL.md
  • src/mapify_cli/templates/skills/map-efficient/efficient-reference.md
  • src/mapify_cli/templates_src/codex/skills/map-efficient/SKILL.md.jinja
  • src/mapify_cli/templates_src/codex/skills/map-efficient/efficient-reference.md.jinja
  • src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja
  • src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
  • src/mapify_cli/templates_src/skills/map-efficient/SKILL.md.jinja
  • src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja
  • tests/_fake_task_tool.py
  • tests/test_concurrent_dispatch_harness.py
  • tests/test_map_orchestrator.py
  • tests/test_map_step_runner.py
  • tests/test_parallelism_observability.py
  • tests/test_project_config.py
  • tests/test_slice5b_leak_guards.py

Comment thread src/mapify_cli/parallelism_observability.py Outdated
Comment thread src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja Outdated
Comment thread src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja Outdated
Comment thread src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja Outdated
Comment thread src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
Comment thread src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
Comment thread src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja Outdated
Comment thread src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
…dings + tests)

F1 phantom_parallel requires max_in_flight<=1; F2 dispatch gate keys on the
ACTIVE wave width (not any wave); F3 lifecycle sidecar serialized via dedicated
file lock; F4 reconcile requires terminal events for every declared subtask;
F5 abort_wave_group preserves group state on failed rollback (head-mismatch);
F6 run_concurrent_wave fails closed on concurrent_dispatch=off; F7 abort-once +
needs_redispatch (no internal merge-retry over discarded worktrees); F8
classifier fed per-subtask base SHAs so isolation_violation is reachable.
@azalio azalio merged commit 77d2335 into main Jun 29, 2026
6 of 7 checks passed
@azalio azalio deleted the feat-303-slice5b-concurrent-dispatch branch June 29, 2026 17:23

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja (1)

2645-2663: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Load step_state.json from project_dir, not CWD.

compute_dispatch_gate(..., project_dir=...) reads config from project_dir, but the active-wave check uses .map/<branch>/step_state.json relative to the process CWD. A caller operating outside the repo root can gate dispatch using the wrong wave state.

🐛 Proposed fix
-    state_file = Path(f".map/{branch}/step_state.json")
+    state_file = Path(project_dir) / ".map" / branch / "step_state.json"
     state = StepState.load(state_file)

Also apply the same project_dir anchoring in select_execution_strategy() where it loads StepState, otherwise concurrency_allowed can still be computed from CWD state.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja` around
lines 2645 - 2663, The active-wave dispatch check is reading StepState from a
path rooted at the current working directory instead of the provided
project_dir, so compute_dispatch_gate can use the wrong step_state.json. Update
the StepState.load path in compute_dispatch_gate to be anchored under
project_dir, and apply the same project_dir-based pathing in
select_execution_strategy where it also loads StepState so concurrency_allowed
is computed from the same repo state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja`:
- Around line 17565-17577: The retry counter in the abort path is being lost
because abort_wave_group() removes the wave group entry before
_read_worktree_state() can update it. Update the abort/retry flow in
map_step_runner.py.jinja so the attempt count is persisted in a separate state
location or captured before calling abort_wave_group(), then use that saved
value when computing attempts_remaining. Keep the logic tied to
abort_wave_group(), _read_worktree_state(), and _write_worktree_state() so
repeated redispatches continue incrementing correctly across failures.

In `@tests/test_map_step_runner.py`:
- Around line 14483-14489: The test around
map_step_runner.record_group_lifecycle is swallowing the expected OSError
instead of asserting propagation. Replace the try/except in this test with
pytest.raises(OSError) so the call to record_group_lifecycle(gk, "ST-Q01",
"started", branch) explicitly verifies the write error is raised while still
covering the release/finally behavior.
- Around line 14169-14199: The test is reimplementing the dispatch SHA
collection logic instead of exercising the real CLI/helper path. Update this
case in test_map_step_runner.py to invoke the actual runner entrypoint or helper
that uses record_dispatch_actual and then assert on the emitted classification
outcome. Keep the assertion tied to classify_dispatch only as a verification
target, but drive it through the real CLI flow so regressions in per-subtask
worktree record reading are caught.
- Around line 14277-14305: The current test only checks the first failure path
and does not verify that retry state persists across redispatches. Update
test_vc3_attempts_remaining_decrements_with_config_max_retries in
test_map_step_runner.py to exercise two consecutive failing calls for the same
wave group by reusing the same run_concurrent_wave / abort_wave_group flow, then
assert that attempts_remaining decrements again (for example from 1 to 0). Use
the existing map_step_runner symbols run_concurrent_wave, merge_wave_worktrees,
and abort_wave_group to confirm the stateful retry counter survives across
abort/redispatch cycles.

---

Outside diff comments:
In `@src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja`:
- Around line 2645-2663: The active-wave dispatch check is reading StepState
from a path rooted at the current working directory instead of the provided
project_dir, so compute_dispatch_gate can use the wrong step_state.json. Update
the StepState.load path in compute_dispatch_gate to be anchored under
project_dir, and apply the same project_dir-based pathing in
select_execution_strategy where it also loads StepState so concurrency_allowed
is computed from the same repo state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ba071137-286a-446a-b385-97fa1c6ad896

📥 Commits

Reviewing files that changed from the base of the PR and between 49ac53f and baed18e.

📒 Files selected for processing (10)
  • .map/scripts/map_orchestrator.py
  • .map/scripts/map_step_runner.py
  • src/mapify_cli/parallelism_observability.py
  • src/mapify_cli/templates/map/scripts/map_orchestrator.py
  • src/mapify_cli/templates/map/scripts/map_step_runner.py
  • src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja
  • src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja
  • tests/test_map_orchestrator.py
  • tests/test_map_step_runner.py
  • tests/test_parallelism_observability.py
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/mapify_cli/parallelism_observability.py
  • tests/test_map_orchestrator.py
  • tests/test_parallelism_observability.py
  • src/mapify_cli/templates/map/scripts/map_orchestrator.py
  • .map/scripts/map_orchestrator.py
  • .map/scripts/map_step_runner.py

Comment on lines +17565 to +17577
abort_wave_group(group_key, branch_name)

# Read attempt count from sidecar (written by begin_wave_group / prior calls).
_st2 = _read_worktree_state(branch_name)
_wg2 = _st2.get("wave_groups") or {}
_grp2 = _wg2.get(group_key) if isinstance(_wg2, dict) else None
_attempts_used = 1
if isinstance(_grp2, dict):
_attempts_used = int(_grp2.get("abort_attempts", 0)) + 1
_grp2["abort_attempts"] = _attempts_used
_write_worktree_state(branch_name, _st2)

attempts_remaining = max(0, max_retries - _attempts_used)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Persist retry attempts outside the group entry before aborting.

abort_wave_group() deletes wave_groups[group_key] on success, so the read at Lines 17567-17575 usually finds no group and resets _attempts_used to 1 every failure. Successive redispatches will not exhaust max_wave_retries.

🐛 Proposed fix
-            abort_wave_group(group_key, branch_name)
-
-            # Read attempt count from sidecar (written by begin_wave_group / prior calls).
+            # Persist attempts outside wave_groups because abort_wave_group removes
+            # the group entry on successful cleanup.
             _st2 = _read_worktree_state(branch_name)
-            _wg2 = _st2.get("wave_groups") or {}
-            _grp2 = _wg2.get(group_key) if isinstance(_wg2, dict) else None
-            _attempts_used = 1
-            if isinstance(_grp2, dict):
-                _attempts_used = int(_grp2.get("abort_attempts", 0)) + 1
-                _grp2["abort_attempts"] = _attempts_used
-                _write_worktree_state(branch_name, _st2)
+            _attempts = _st2.setdefault("wave_group_abort_attempts", {})
+            if not isinstance(_attempts, dict):
+                _attempts = {}
+                _st2["wave_group_abort_attempts"] = _attempts
+            _attempts_used = int(_attempts.get(group_key, 0)) + 1
+            _attempts[group_key] = _attempts_used
+            _write_worktree_state(branch_name, _st2)
+
+            abort_wave_group(group_key, branch_name)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
abort_wave_group(group_key, branch_name)
# Read attempt count from sidecar (written by begin_wave_group / prior calls).
_st2 = _read_worktree_state(branch_name)
_wg2 = _st2.get("wave_groups") or {}
_grp2 = _wg2.get(group_key) if isinstance(_wg2, dict) else None
_attempts_used = 1
if isinstance(_grp2, dict):
_attempts_used = int(_grp2.get("abort_attempts", 0)) + 1
_grp2["abort_attempts"] = _attempts_used
_write_worktree_state(branch_name, _st2)
attempts_remaining = max(0, max_retries - _attempts_used)
# Persist attempts outside wave_groups because abort_wave_group removes
# the group entry on successful cleanup.
_st2 = _read_worktree_state(branch_name)
_attempts = _st2.setdefault("wave_group_abort_attempts", {})
if not isinstance(_attempts, dict):
_attempts = {}
_st2["wave_group_abort_attempts"] = _attempts
_attempts_used = int(_attempts.get(group_key, 0)) + 1
_attempts[group_key] = _attempts_used
_write_worktree_state(branch_name, _st2)
abort_wave_group(group_key, branch_name)
attempts_remaining = max(0, max_retries - _attempts_used)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja` around
lines 17565 - 17577, The retry counter in the abort path is being lost because
abort_wave_group() removes the wave group entry before _read_worktree_state()
can update it. Update the abort/retry flow in map_step_runner.py.jinja so the
attempt count is persisted in a separate state location or captured before
calling abort_wave_group(), then use that saved value when computing
attempts_remaining. Keep the logic tied to abort_wave_group(),
_read_worktree_state(), and _write_worktree_state() so repeated redispatches
continue incrementing correctly across failures.

Comment on lines +14169 to +14199
# Replicate the runner CLI's per-subtask SHA collection logic (record_dispatch_actual).
group_sids = group_data.get("subtask_ids", [])
group_level_sha = group_data.get("base_sha")
base_shas: list[str] = []
for sid in group_sids:
slug = map_step_runner._wt_slug(sid)
wt_rec = worktrees.get(slug) if (isinstance(worktrees, dict) and slug) else None
if isinstance(wt_rec, dict):
per_sha = wt_rec.get("base_sha")
if isinstance(per_sha, str) and per_sha:
base_shas.append(per_sha)
continue
if isinstance(group_level_sha, str) and group_level_sha:
base_shas.append(group_level_sha)

# Two members with different SHAs → classify_dispatch must return isolation_violation.
assert len(set(base_shas)) > 1, (
f"Expected >1 distinct base_shas to trigger isolation_violation; "
f"got base_shas={base_shas!r}"
)
from mapify_cli.parallelism_observability import (
classify_dispatch as _classify,
DISPATCH_OUTCOME_ISOLATION_VIOLATION,
)
outcome = _classify(
same_turn_task_count=2,
max_in_flight=2,
base_shas=base_shas,
skill_reported_concurrent=True,
)
assert outcome == DISPATCH_OUTCOME_ISOLATION_VIOLATION, (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

This doesn't exercise the CLI path it claims to cover.

The test rebuilds base_shas inline and calls classify_dispatch() directly, so it will still pass if record_dispatch_actual regresses and stops reading per-subtask worktree records. Please invoke the actual runner CLI/helper and assert on its emitted classification instead of reimplementing the logic in the test.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_map_step_runner.py` around lines 14169 - 14199, The test is
reimplementing the dispatch SHA collection logic instead of exercising the real
CLI/helper path. Update this case in test_map_step_runner.py to invoke the
actual runner entrypoint or helper that uses record_dispatch_actual and then
assert on the emitted classification outcome. Keep the assertion tied to
classify_dispatch only as a verification target, but drive it through the real
CLI flow so regressions in per-subtask worktree record reading are caught.

Comment on lines +14277 to +14305
def test_vc3_attempts_remaining_decrements_with_config_max_retries(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""VC3: attempts_remaining decrements; with max_retries=2 first call → remaining=1."""
def _always_fail(ids: list[str], branch: Any = None, **kw: Any) -> dict[str, Any]:
del ids, branch, kw
return {"status": "error", "ok": False, "kind": "WAVE_MERGE_CONFLICT", "message": "x"}

def _fake_abort(group_id: str, branch: Any = None) -> dict[str, Any]:
del group_id, branch
return {"clean": True, "aborted_group_id": "fake"}

monkeypatch.setattr(map_step_runner, "merge_wave_worktrees", _always_fail)
monkeypatch.setattr(map_step_runner, "abort_wave_group", _fake_abort)

repo = self._make_repo_with_dispatch_enabled(tmp_path, max_retries=2)
monkeypatch.chdir(repo)
monkeypatch.setattr(map_step_runner, "get_branch_name", lambda: "test-branch")

result = map_step_runner.run_concurrent_wave(
["ST-001", "ST-002"], "test-branch", repo
)

assert result.get("kind") == "WAVE_ABORTED"
assert result.get("needs_redispatch") is True
# First call uses attempt 1 of 2; 1 attempt remaining.
assert result.get("attempts_remaining") == 1, (
f"expected attempts_remaining=1 after first failure; got {result}"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

This test never proves retry state survives across redispatches.

run_concurrent_wave() updates abort_attempts only after abort_wave_group(), and abort_wave_group() removes the group entry on a normal abort. That makes the cross-call decrement path fragile, but this test only asserts the first failure. Please drive two failing calls against the same group and assert the counter drops again (for example 1 -> 0) so the stateful retry contract is actually covered.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_map_step_runner.py` around lines 14277 - 14305, The current test
only checks the first failure path and does not verify that retry state persists
across redispatches. Update
test_vc3_attempts_remaining_decrements_with_config_max_retries in
test_map_step_runner.py to exercise two consecutive failing calls for the same
wave group by reusing the same run_concurrent_wave / abort_wave_group flow, then
assert that attempts_remaining decrements again (for example from 1 to 0). Use
the existing map_step_runner symbols run_concurrent_wave, merge_wave_worktrees,
and abort_wave_group to confirm the stateful retry counter survives across
abort/redispatch cycles.

Comment on lines +14483 to +14489
# The write will raise — record_group_lifecycle should propagate the error
# but release must still have been called.
try:
map_step_runner.record_group_lifecycle(gk, "ST-Q01", "started", branch)
except OSError:
pass # expected — the write is patched to raise

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the write error instead of swallowing it.

This currently verifies the finally path, but not the stated propagation contract. If record_group_lifecycle() starts catching the OSError, the test still passes. Wrap the call in pytest.raises(OSError) so the test checks both behaviors.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_map_step_runner.py` around lines 14483 - 14489, The test around
map_step_runner.record_group_lifecycle is swallowing the expected OSError
instead of asserting propagation. Replace the try/except in this test with
pytest.raises(OSError) so the call to record_group_lifecycle(gk, "ST-Q01",
"started", branch) explicitly verifies the write error is raised while still
covering the release/finally behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant